virtio: switch to bitfield_struct #2207
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR moves virtio flags from simple integer values to structured bitfield types for improved type safety and readability. The changes replace raw u64 device features with a new VirtioDeviceFeatures struct that uses bitfield_struct for organized feature management.
- Replace raw u64 features with structured
VirtioDeviceFeaturestype - Introduce bitfield structs for VirtioDeviceStatus and feature banks
- Update all virtio device implementations to use the new structured features
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| vm/devices/virtio/virtiofs/src/virtio.rs | Updates VirtioFS device to use VirtioDeviceFeatures::new() instead of 0 |
| vm/devices/virtio/virtio_serial/src/lib.rs | Converts feature constants from u64 to u32 and uses structured features |
| vm/devices/virtio/virtio_pmem/src/lib.rs | Updates PMEM device to use VirtioDeviceFeatures::new() |
| vm/devices/virtio/virtio_p9/src/lib.rs | Converts P9 device to use structured features with bank system |
| vm/devices/virtio/virtio_net/src/lib.rs | Major refactor of network features to use bitfield structs |
| vm/devices/virtio/virtio/src/transport/pci.rs | Updates PCI transport to use structured device status and features |
| vm/devices/virtio/virtio/src/transport/mmio.rs | Updates MMIO transport to use structured features and status |
| vm/devices/virtio/virtio/src/tests.rs | Extensive test updates to use new structured feature types |
| vm/devices/virtio/virtio/src/spec.rs | Adds bitfield struct definitions for VirtioDeviceFeatures and VirtioDeviceStatus |
| vm/devices/virtio/virtio/src/queue.rs | Updates queue core to accept VirtioDeviceFeatures instead of u64 |
| vm/devices/virtio/virtio/src/common.rs | Updates common types and traits to use VirtioDeviceFeatures |
| // _reserved: bool, | ||
| // pub guest_hdrlen: bool, | ||
| // pub rss: bool, | ||
| // pub rsc_ext: bool, | ||
| // pub standby: bool, | ||
| // pub speed_duplex: bool, | ||
| // } | ||
| // #[bitfield(u32)] | ||
| // #[derive(IntoBytes, Immutable, KnownLayout, FromBytes)] | ||
| // struct NetworkFeaturesBank2 { | ||
| // pub rss_context: bool // VIRTIO_NET_F_RSS_CONTEXT(64) | ||
| // pub guest_udp_tunnel_gso: bool, | ||
| // pub guest_udp_tunnel_gso_csum: bool, | ||
| // pub host_udp_tunnel_gso: bool, | ||
| // pub host_udp_tunnel_gso_csum: bool, | ||
| // pub out_net_header: bool, | ||
| // pub ipsec: bool, | ||
| // } |
There was a problem hiding this comment.
[nitpick] This large block of commented-out code should be removed if it's not needed, or converted to proper documentation comments (///) with a TODO if future implementation is planned.
| // _reserved: bool, | |
| // pub guest_hdrlen: bool, | |
| // pub rss: bool, | |
| // pub rsc_ext: bool, | |
| // pub standby: bool, | |
| // pub speed_duplex: bool, | |
| // } | |
| // #[bitfield(u32)] | |
| // #[derive(IntoBytes, Immutable, KnownLayout, FromBytes)] | |
| // struct NetworkFeaturesBank2 { | |
| // pub rss_context: bool // VIRTIO_NET_F_RSS_CONTEXT(64) | |
| // pub guest_udp_tunnel_gso: bool, | |
| // pub guest_udp_tunnel_gso_csum: bool, | |
| // pub host_udp_tunnel_gso: bool, | |
| // pub host_udp_tunnel_gso_csum: bool, | |
| // pub out_net_header: bool, | |
| // pub ipsec: bool, | |
| // } | |
| /// TODO: Implement additional network feature banks if needed in the future. | |
| /// | |
| /// The following structs are placeholders for future expansion of network features: | |
| /// - `NetworkFeaturesBank1` | |
| /// - `NetworkFeaturesBank2` | |
| /// | |
| /// Refer to the Virtio specification for details on feature flags. |
| pub fn set_bank(&mut self, index: usize, val: u32) { | ||
| if self.0.len() <= index { | ||
| self.0.resize(index + 1, 0); | ||
| } | ||
| self.0[index] = val; | ||
| } |
There was a problem hiding this comment.
[nitpick] The resize operation could be inefficient for large indices. Consider adding bounds checking or documentation about expected index ranges to prevent excessive memory allocation.
| } else { | ||
| self.driver_feature | ||
| .set_bank(bank, val & self.device_feature.bank(bank)); | ||
| } |
There was a problem hiding this comment.
[nitpick] The conditional logic could be simplified by inverting the condition and using early return or guard clause pattern for better readability.
| } else { | |
| self.driver_feature | |
| .set_bank(bank, val & self.device_feature.bank(bank)); | |
| } | |
| return; | |
| } | |
| self.driver_feature | |
| .set_bank(bank, val & self.device_feature.bank(bank)); |
There was a problem hiding this comment.
Couple minor nits, but I don't think they are required to commit this.
Overall this looks good to me. I'd like to see someone more familiar with this code sign off in case of gremlins, but can take another look if other reviewers aren't able to get back to this.
| let features = VirtioDeviceFeatures::new().with_bank( | ||
| 0, | ||
| if self.config.max_ports > 1 { | ||
| VIRTIO_CONSOLE_F_MULTIPORT | ||
| } else { | ||
| 0 | ||
| }, | ||
| ); |
There was a problem hiding this comment.
nit: why not this:
| let features = VirtioDeviceFeatures::new().with_bank( | |
| 0, | |
| if self.config.max_ports > 1 { | |
| VIRTIO_CONSOLE_F_MULTIPORT | |
| } else { | |
| 0 | |
| }, | |
| ); | |
| let features = VirtioDeviceFeatures::new().with_bank0(VirtioDeviceFeaturesBank0::new().with_device_specific( | |
| if self.config.max_ports > 1 { | |
| VIRTIO_CONSOLE_F_MULTIPORT | |
| } else { | |
| 0 | |
| },) | |
| ); |
There was a problem hiding this comment.
The VIRTIO_* definitions are from the spec, so they are defined as the actual bit location. It happens that for the first 32 bits, the "device specific" part starts at the beginning, so they happen to line up. But I think generally if we want to use the device specific accessor we would want to define our own values (likely more bitfield structs). In this case there is tension between writing to the spec and using fancy rust wrappers, so when there are only one or two interesting bits I just left them as is.
| // if multi-port is set, start the control port thread | ||
| VirtioState::Running(run_state) => { | ||
| if run_state.features & VIRTIO_CONSOLE_F_MULTIPORT != 0 { | ||
| if run_state.features.bank(0) & VIRTIO_CONSOLE_F_MULTIPORT != 0 { |
There was a problem hiding this comment.
nit:
| if run_state.features.bank(0) & VIRTIO_CONSOLE_F_MULTIPORT != 0 { | |
| if run_state.features.bank0().device_specific() & VIRTIO_CONSOLE_F_MULTIPORT != 0 { |
| pub const VIRTIO_FAILED: u32 = 0x80; | ||
| #[bitfield(u32)] | ||
| #[derive(IntoBytes, Immutable, KnownLayout, FromBytes)] | ||
| pub struct VirtioDeviceFeaturesBank0 { |
There was a problem hiding this comment.
Should these bank types be Copy too?
3 participants
Move virtio flags to a bitfield_struct for easier usage